Skip to content

[SPARK-57883][CORE][TESTS] Fix PluginContainerSuite to check executor memory correctly#56964

Closed
dongjoon-hyun wants to merge 1 commit into
apache:masterfrom
dongjoon-hyun:SPARK-57883
Closed

[SPARK-57883][CORE][TESTS] Fix PluginContainerSuite to check executor memory correctly#56964
dongjoon-hyun wants to merge 1 commit into
apache:masterfrom
dongjoon-hyun:SPARK-57883

Conversation

@dongjoon-hyun

@dongjoon-hyun dongjoon-hyun commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This PR aims to fix PluginContainerSuite to check executor memory correctly.

Why are the changes needed?

Previously, it checks driver accidentally because it didn't filter out driver from the executor list.

spark/core/src/test/scala/org/apache/spark/internal/plugin/PluginContainerSuite.scala

Lines 261 to 263 in e3e6d9e

// Check executor memory is also updated
val execInfo = sc.statusTracker.getExecutorInfos.head
assert(execInfo.totalOffHeapStorageMemory() == MemoryOverridePlugin.offHeapMemory)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the CIs.

Was this patch authored or co-authored using generative AI tooling?

No.

@dongjoon-hyun

dongjoon-hyun commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Could you review this PR when you have some time, @viirya ? I decided to fix this bug first from PluginContainerSuite before moving forward the original PR.

viirya
viirya approved these changes Jul 2, 2026
View reviewed changes

@viirya viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Verified the root cause independently: getExecutorInfos goes through AppStatusStore.executorList(true), which reads base.index("active").reverse().first(true).last(true) — within the index, entries sort by executor id, so after reverse() the "driver" entry (letters sort after digits) comes first and .head was indeed always the driver. The old assertion only stayed green because the driver happened to carry the same off-heap value, so the executor side was never actually verified.

The eventually is necessary rather than just defensive: waitUntilExecutorsUp only guarantees the executor entry exists (from onExecutorAdded), while memoryMetrics is populated later when the executor's BlockManager registers, so the first read can legitimately see None.

With this fixed, the test genuinely verifies that plugin-set off-heap settings reach the executors — which also makes it a useful guard for #56945.

@dongjoon-hyun

dongjoon-hyun commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Thank you, @viirya !

@dongjoon-hyun

dongjoon-hyun commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Since this is a test suite only change and I verified locally like the following, I'm going to merge.

$ build/sbt "core/testOnly *.PluginContainerSuite"
...
[info] PluginContainerSuite:
[info] - plugin initialization and communication (1 second, 156 milliseconds)
[info] - do nothing if plugins are not configured (65 milliseconds)
[info] - merging of config options (33 milliseconds)
[info] - SPARK-33088: executor tasks trigger plugin calls (435 milliseconds)
[info] - SPARK-33088: executor failed tasks trigger plugin calls (57 milliseconds)
[info] - plugin initialization in non-local mode (1 second, 753 milliseconds)
[info] - plugin initialization in non-local mode with resources (1 second, 464 milliseconds)
[info] - memory override in plugin (1 second, 487 milliseconds)
[info] - The plugin should be shutdown before the listener bus is stopped (25 milliseconds)
[info] - SPARK-52548: override shuffle manager in plugin (21 milliseconds)
[info] Run completed in 7 seconds, 490 milliseconds.
[info] Total number of tests run: 10
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 10, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 12 s, completed Jul 2, 2026, 9:43:17 AM

dongjoon-hyun added a commit that referenced this pull request Jul 2, 2026
@dongjoon-hyun
[SPARK-57883][CORE][TEST] Fix PluginContainerSuite to check executo…
8cadba0 
…r memory correctly

### What changes were proposed in this pull request?

This PR aims to fix `PluginContainerSuite` to check executor memory correctly.

### Why are the changes needed?

Previously, it checks `driver` accidentally because it didn't filter out driver from the executor list.

https://github.com/apache/spark/blob/e3e6d9eb31012244692c043eb92a582ae3c3751b/core/src/test/scala/org/apache/spark/internal/plugin/PluginContainerSuite.scala#L261-L263

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #56964 from dongjoon-hyun/SPARK-57883.

Authored-by: Dongjoon Hyun <dongjoon@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 17e7e14)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@dongjoon-hyun

dongjoon-hyun commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Merge Summary:

Posted by merge_spark_pr.py

@dongjoon-hyun dongjoon-hyun deleted the SPARK-57883 branch July 2, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dongjoon-hyun @viirya